Skip to content

HBASE-27883: Use log4j2 instead of log4j for logging.#117

Merged
Reidddddd merged 1 commit intoapache:masterfrom
subrat-mishra:HBASE-27883
Aug 10, 2023
Merged

HBASE-27883: Use log4j2 instead of log4j for logging.#117
Reidddddd merged 1 commit intoapache:masterfrom
subrat-mishra:HBASE-27883

Conversation

@subrat-mishra
Copy link

@subrat-mishra subrat-mishra commented May 29, 2023

No description provided.

@Apache-HBase

This comment was marked as outdated.

Sign in to view
@petersomogyi petersomogyi mentioned this pull request Aug 2, 2023
Closed
@Reidddddd
Copy link

Reidddddd commented Aug 3, 2023

re-trigger the check

@Apache-HBase

This comment was marked as outdated.

Sign in to view
Reidddddd
Reidddddd previously approved these changes Aug 3, 2023
View reviewed changes
Copy link

@Reidddddd Reidddddd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Reidddddd
Copy link

Reidddddd commented Aug 3, 2023

emm, hold on

just checked the code base, it seems using slf4j not apache logging...

@Reidddddd Reidddddd dismissed their stale review August 3, 2023 05:40

Need double checks

@subrat-mishra
Copy link
Author

subrat-mishra commented Aug 5, 2023

re-trigger the check

@Apache-HBase

This comment was marked as outdated.

Sign in to view
@subrat-mishra
Copy link
Author

subrat-mishra commented Aug 7, 2023

@Reidddddd, could you please take a look at it? I have removed slf4j-log4j12 from hbase-spark/pom.xml and also the transitive dependencies as well. Is it fine now?

@Reidddddd
Copy link

Reidddddd commented Aug 7, 2023

ping @petersomogyi, would you like to give a review as well

@Reidddddd
Copy link

Reidddddd commented Aug 8, 2023

@subrat-mishra there are places i don't understand, could you explain them to me

  1. why replace log4j with org.apache.logging.log4j
  2. and org.apache.logging.log4j is still log4j not log4j2? seems not as title describes

@petersomogyi
Copy link
Contributor

petersomogyi commented Aug 8, 2023

  1. why replace log4j with org.apache.logging.log4j

That is the groupId for log4j2, check these dependencies for the hbase repository: https://github.com/apache/hbase/blob/master/pom.xml#L1340-L1359

  1. and org.apache.logging.log4j is still log4j not log4j2? seems not as title describes

org.apache.logging.log4j is with version 2.17.2, so it is log4j2.

@petersomogyi
Copy link
Contributor

petersomogyi commented Aug 8, 2023

Should the org.slf4j dependencies be updated? For example slf4j-log4j12 is binding/provider for log4j version 1.2, shouldn't be needed anymore.
SLF4J dependencies in hbase: https://github.com/apache/hbase/blob/master/pom.xml#L1325-L1339

@subrat-mishra
Copy link
Author

subrat-mishra commented Aug 8, 2023

Should the org.slf4j dependencies be updated? For example slf4j-log4j12 is binding/provider for log4j version 1.2, shouldn't be needed anymore. SLF4J dependencies in hbase: https://github.com/apache/hbase/blob/master/pom.xml#L1325-L1339

Thanks @petersomogyi, for pointing that out. I missed removing slf4j-log4j12 from parent pom.xml

@Apache-HBase
Copy link

Apache-HBase commented Aug 8, 2023

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 9s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-0 ⚠️ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+0 🆗 mvndep 0m 19s Maven dependency ordering for branch
+1 💚 mvninstall 1m 20s master passed
+1 💚 compile 0m 52s master passed
+1 💚 javadoc 0m 51s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for patch
+1 💚 mvninstall 0m 49s the patch passed
+1 💚 compile 0m 51s the patch passed
+1 💚 javac 0m 51s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 xml 0m 3s The patch has no ill-formed XML file.
+1 💚 javadoc 0m 49s the patch passed
_ Other Tests _
+1 💚 unit 8m 3s root in the patch passed.
15m 13s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-117/6/artifact/yetus-precommit-check/output/Dockerfile
GITHUB PR #117
Optional Tests dupname javac javadoc unit xml compile
uname Linux 6de5c9c68dab 5.4.0-148-generic #165-Ubuntu SMP Tue Apr 18 08:53:12 UTC 2023 x86_64 GNU/Linux
Build tool hb_maven
Personality dev-support/jenkins/hbase-personality.sh
git revision master / bfcdf68
Default Java Oracle Corporation-1.8.0_282-b08
Test Results https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-117/6/testReport/
Max. process+thread count 936 (vs. ulimit of 12500)
modules C: kafka/hbase-kafka-proxy spark/hbase-spark spark/hbase-spark-it . U: .
Console output https://ci-hbase.apache.org/job/HBase-Connectors-PreCommit/job/PR-117/6/console
versions git=2.20.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@petersomogyi petersomogyi requested a review from Reidddddd August 9, 2023 12:19
@Reidddddd
Copy link

Reidddddd commented Aug 10, 2023

That is the groupId for log4j2, check these dependencies for the hbase repository: https://github.com/apache/hbase/blob/master/pom.xml#L1340-L1359

not familiar with these parts, thanks for explanation

@Reidddddd Reidddddd merged commit 35f962c into apache:master Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petersomogyi petersomogyi petersomogyi approved these changes

@Reidddddd Reidddddd Reidddddd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@subrat-mishra @Apache-HBase @Reidddddd @petersomogyi